Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Explicit tgv backend #1997

Merged
merged 11 commits into from
May 1, 2024
Merged

Explicit tgv backend #1997

merged 11 commits into from
May 1, 2024

Conversation

samtygier-stfc
Copy link
Collaborator

@samtygier-stfc samtygier-stfc commented Dec 13, 2023 •
edited
Loading

I made a branch of #1986 within the MI repo so that the PR can access the secrets on actions (I can't see a simple way to give the fork access).

Issue

closes #1985

Description

Implementation of a Total Generalised Variation regulariser for Least Squares problems. It works both with PDHG and the SPDHG algorithms.

Controls to from the interface that are required:

  1. alpha, regularisation parameter as in Eq. 3.5 https://royalsocietypublishing.org/doi/10.1098/rsta.2020.0193
  2. gamma, ratio beta/alpha, wrt to Eq. 3.5 https://royalsocietypublishing.org/doi/10.1098/rsta.2020.0193
  3. regulariser, whether TV or TGV

Testing

Edit ReconstructWindowView.regulariser to return "TGV". In the reconstruction window select CIL:PDHG-TV, and check that it still does a reconstruction.

Documentation

Wait for the GUI side

@samtygier-stfc samtygier-stfc force-pushed the 1986-explicit_TGV branch 2 times, most recently from 58a81bb to a4c5b5e Compare December 13, 2023 17:57
@coveralls
Copy link

coveralls commented Dec 13, 2023 •
edited
Loading

Coverage Status

coverage: 73.005% (-0.4%) from 73.358%
when pulling 0d07c18 on 1986-explicit_TGV
into b370ba9 on main.

@samtygier-stfc samtygier-stfc marked this pull request as ready for review April 23, 2024 08:25
@samtygier-stfc samtygier-stfc marked this pull request as draft April 23, 2024 08:38
@samtygier-stfc samtygier-stfc marked this pull request as ready for review April 23, 2024 09:22
@samtygier-stfc samtygier-stfc mentioned this pull request Apr 23, 2024
Closed
@MikeSullivan7 MikeSullivan7 self-assigned this Apr 23, 2024
@MikeSullivan7
Copy link
Collaborator

Selecting CIL:PDHG-TV after changing the regulariser to TGV doesnt seem to produce a reconstruction preview, i.e.

image

and the reconstruction just gives a blank reconstruction.

I will do a deeper investigation to figure out what exactly is going on

@samtygier-stfc
Copy link
Collaborator Author

Do you get anything if you increase the number of iterations?

@MikeSullivan7
Copy link
Collaborator

Ah yes, I didnt put the number of iterations high enough, my bad!

@MikeSullivan7
Copy link
Collaborator

After using CIL: PDHG-TV with "TGV" selected and toggling the "stochastic" option, I get an error:

image

2024-04-25 09:42:30,166 [mantidimaging.gui.dialogs.async_task.task:L53] ERROR: Failed to execute task: tuple index out of range
Traceback (most recent call last):
  File "C:\Users\ddb29996\mantidimaging\mantidimaging\gui\dialogs\async_task\task.py", line 50, in run
    self.result = call_with_known_parameters(self.task_function, **self.kwargs)
  File "C:\Users\ddb29996\mantidimaging\mantidimaging\core\utility\func_call.py", line 12, in call_with_known_parameters
    return func(**ka)
  File "C:\Users\ddb29996\mantidimaging\mantidimaging\gui\windows\recon\model.py", line 132, in run_preview_recon
    recon.data[0] = reconstructor.single_sino(images.sino(slice_idx),
  File "C:\Users\ddb29996\mantidimaging\mantidimaging\core\reconstruct\cil_recon.py", line 249, in single_sino
    algo = SPDHG(f=F,
  File "C:\Users\ddb29996\AppData\Local\miniforge3\envs\mantidimaging-dev\lib\site-packages\cil\optimisation\algorithms\SPDHG.py", line 105, in __init__
    self.set_up(f=f, g=g, operator=operator, tau=tau, sigma=sigma,
  File "C:\Users\ddb29996\AppData\Local\miniforge3\envs\mantidimaging-dev\lib\site-packages\cil\optimisation\algorithms\SPDHG.py", line 156, in set_up
    norms = [operator.get_item(i,0).norm() for i in range(self.ndual_subsets)]
  File "C:\Users\ddb29996\AppData\Local\miniforge3\envs\mantidimaging-dev\lib\site-packages\cil\optimisation\algorithms\SPDHG.py", line 156, in <listcomp>
    norms = [operator.get_item(i,0).norm() for i in range(self.ndual_subsets)]
  File "C:\Users\ddb29996\AppData\Local\miniforge3\envs\mantidimaging-dev\lib\site-packages\cil\optimisation\operators\BlockOperator.py", line 136, in get_item
    return self.operators[index]
IndexError: tuple index out of range
2024-04-25 09:42:30,168 [mantidimaging.gui.mvp_base.view:L46] ERROR: show_error_dialog(): Encountered error while trying to reconstruct: tuple index out of range

Is this expected?

@samtygier-stfc
Copy link
Collaborator Author

Its expected not to work :-). The GUI (when it is done), will block setting stochastic for TGV. But we should probably have a better error message here.

MikeSullivan7
MikeSullivan7 approved these changes Apr 25, 2024
View reviewed changes
Copy link
Collaborator

@MikeSullivan7 MikeSullivan7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, seems to match up with the paper and give good results with a higher number of iterations

@MikeSullivan7 MikeSullivan7 added this pull request to the merge queue Apr 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 25, 2024
@MikeSullivan7 MikeSullivan7 added this pull request to the merge queue May 1, 2024
Merged via the queue into main with commit 9d23fb7 May 1, 2024
8 checks passed
@MikeSullivan7 MikeSullivan7 deleted the 1986-explicit_TGV branch May 1, 2024 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MikeSullivan7 MikeSullivan7 MikeSullivan7 approved these changes

Assignees

@MikeSullivan7 MikeSullivan7

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add TGV regularised LeastSquares reconstruction
4 participants
@samtygier-stfc @coveralls @MikeSullivan7 @paskino